-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature react native tracer provider #53
base: main
Are you sure you want to change the base?
Conversation
…vider-export-package-setup EMBR-3907 package setup
…er-integration-test-setup
…oid-generated-files rename native-tests -> native-src
…-native-tracer-provider
…-native-tracer-provider
Dependency ReviewThe following issues were found:
|
…tations for boolean attributes (#153)
@@ -0,0 +1,5 @@ | |||
TracerProvider_kotlinVersion=1.7.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason why we need a prefix here? TracerProvider_
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had copied and pasted this from another example, I believe the reasoning is to distinguish it from another "kotlinVersion" property that may have been defined by the app that uses this package, e.g. in packages/react-native-tracer-provider/android/build.gradle
we have:
def kotlin_version = rootProject.ext.has("kotlinVersion") ? rootProject.ext.get("kotlinVersion") : project.properties["TracerProvider_kotlinVersion"]
So we use a bare "kotlinVersion" property if it's available and then only fallback to this "TracerProvider_kotlinVersion" if it's not
@@ -0,0 +1,4 @@ | |||
dependencies { | |||
api "io.embrace:embrace-android-sdk:6.13.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to update this version manually here in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this ends up getting rewritten by the update_android_version.ts
script
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can cleanup all these files, I think everything into this mipmap-hdpi
was auto generated, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep they were all created when I initialized the react native app, I can double-check what happens if I delete the folder, not sure if any are required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like most aren't required, deleted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in my react-native-otlp
package I ended up creating a file called RNEmbraceOTLP.podspec. maybe we can agree on a naming convention for this type of files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya let's discuss this one a bit more, I when with the react-native-tracer-provider.podspec
to better match the package name though since we have RNEmbrace.podspec
in core it is a bit inconsistent
* | ||
* The JS side of its implementation is modelled after [opentelemetry-sdk-trace-base](https://github.com/open-telemetry/opentelemetry-js/tree/main/packages/opentelemetry-sdk-trace-base) | ||
*/ | ||
export const useEmbraceNativeTracerProvider = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we move the hook into a different place? as per the name of this file (EmbraceNativeTracerProvider
) I though I was going to see only a class but it is also containing the hook. I would suggest to move the hook to its own place useEmbraceNativeTracerProvider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, moved to its own useEmbraceNativeTracerProvider.ts
file
@@ -0,0 +1,22 @@ | |||
import {TracerProvider} from "@opentelemetry/api"; | |||
|
|||
export interface UseEmbraceNativeTracerProviderResult { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we remove the Use
prefix from the name for this interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure about the convention here, this is the type of the return value of useEmbraceNativeTracerProvider
so I was naming it to match, what do you think?
Feature branch for the react-native-tracer-provider package
Before merging:
main
After merging: